Add Ferrari-Cardano algorithm and general Quartic Solver test suite #2010
Conversation
… required higher precision than tolerance allowed
…fixed backwards iterator, fixed a couple of arithmetic bugs
…ss an implementation of that
Test summary 5 750 files 9 236 suites 18m 7s ⏱️ Results for commit 1221aa8. ♻️ This comment has been updated with latest results. |
sethrj
added
enhancement
There was a problem hiding this comment.
Pull Request Overview
This pull request adds a Ferrari-Cardano method implementation for solving quartic polynomial equations and a comprehensive test suite. The implementation is designed to find positive, real, non-zero roots for quartic functions used in geometric surface calculations.
Key changes include:
- Implementation of the Ferrari-Cardano algorithm in
FerrariSolverclass - Comprehensive test framework for quartic solvers with abstract base class design
- Addition of CMake preset configurations for development workflows
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/orange/surf/detail/FerrariSolver.hh |
Core implementation of Ferrari-Cardano quartic solver with cubic and quadratic utility functions |
test/orange/surf/detail/QuarticSolver.test.cc |
Abstract test framework and specific Ferrari solver tests covering various root scenarios |
test/orange/CMakeLists.txt |
Registration of new quartic solver test |
scripts/cmake-presets/.json |
CMake preset configurations for development builds |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
elliottbiondo
left a comment
elliottbiondo
left a comment
There was a problem hiding this comment.
I think this is basically ready to merge: just a few minor comments
sethrj
left a comment
sethrj
left a comment
There was a problem hiding this comment.
Looks great Owen! Just a few final comments based on the changes.
…ly use the Cardano method as its cubic rootfinder
…nd made normal operator call the surface operator when surface-start is specified
…ll be that surface solve will simply call four coefficient operator
|
@elliottbiondo I think this is ready to merge! |
|
@osanstrong @elliottbiondo Since we've made the solver more generic, I've moved it to its rightful place in |
This reverts commit 303a17b.
|
@elliottbiondo @osanstrong Please take a quick look at my changes and then we can merge: |
elliottbiondo
left a comment
elliottbiondo
left a comment
There was a problem hiding this comment.
These changes look good to me. Let's wait for @osanstrong to take a look before merging.
|
@elliottbiondo I'm going to merge this unless @osanstrong says otherwise. Thanks guys! Next step, basic torus support? |
|
Looks good to me! (sorry for not getting back to you faster) Only question I had atp was if you think it's generic enough to be in corecel, should it also return negative roots and let the torus code ignore them? Thanks for the last edits! |
|
I thought about that, and if we ever need nonpositive roots, we can easily make it more generic by constructing with a "filter" template argument. |
3 participants
This pull request adds FerrariSolver, a class implementing the Ferrari-Cardano method to solve quartic polynomials of the form
ax⁴ + bx³ + cx² + dx + e = 0, and QuarticSolver.test, a class with tests for quartic solvers.The Ferrari-Cardano method solves a related cubic equation to find a factor which breaks the quartic equation into two quadratic equations, which are then solved for the final roots.
The QuarticSolver test file contains a brief abstract framework for testing quartic functions, and a specific subclass FerrariSolverTest which plugs in FerrariSolver for those tests. Most of the tests can then be easily reused to test an alternative quartic solver, with a different subclass.